Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable CA1822 (Mark members as static) analyzer #66333

Merged
merged 5 commits into from
Mar 25, 2022

Conversation

marek-safar
Copy link
Contributor

No description provided.

@marek-safar marek-safar requested a review from a team March 8, 2022 13:29
@marek-safar marek-safar marked this pull request as draft March 8, 2022 13:29
@ghost ghost assigned marek-safar Mar 8, 2022
@ghost
Copy link

ghost commented Mar 8, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: marek-safar
Assignees: marek-safar
Labels:

area-Meta

Milestone: -

public void TraceNode(QilNode n)
{
#if QIL_TRACE_NODE_CREATION
this.diag.AddNode(n);
#endif
}
#pragma warning restore CA1822
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krwq, is any of this tracing stuff ever used? Wondering if we can just delete it all instead.

@marek-safar marek-safar force-pushed the CA1822 branch 17 times, most recently from dcf6f93 to 2c165a0 Compare March 11, 2022 16:57
@@ -710,15 +710,15 @@ private void OnIndexedHeaderName(int index)
_state = State.HeaderValueLength;
}

private void OnIndexedHeaderNamePostBase(int index)
private static void OnIndexedHeaderNamePostBase(int index)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These won't be static once they're actually implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine, one you implemented them you remove static

@@ -10,9 +10,9 @@ internal sealed class DefaultSettingsSection // ConfigurationSection

internal static DefaultSettingsSection GetSection() => s_section;

public string DistributedTransactionManagerName { get; set; } = ConfigurationStrings.DefaultDistributedTransactionManagerName;
public static string DistributedTransactionManagerName { get; set; } = ConfigurationStrings.DefaultDistributedTransactionManagerName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roji should weigh in on whether any of these changes are going to conflict with his work.

@@ -710,15 +710,15 @@ private void OnIndexedHeaderName(int index)
_state = State.HeaderValueLength;
}

private void OnIndexedHeaderNamePostBase(int index)
private static void OnIndexedHeaderNamePostBase(int index)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this file is mirrored with aspnetcore. It'd be good to validate whether it's going to break anything when that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be fine as it's private and aspnetcore already had them static (they have CS1822 enabled) before sync with runtime removed it.

dotnet/aspnetcore#39453

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the remaining comments, LGTM. (I don't plan to re-review the 642 files again ;-)

@marek-safar marek-safar merged commit 6a889d2 into dotnet:main Mar 25, 2022
@marek-safar marek-safar deleted the CA1822 branch March 25, 2022 11:06
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 24, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants